Skip to content

fix: render safeHTML in toasts#769

Open
RitvikSardana wants to merge 1 commit into
frappe:mainfrom
RitvikSardana:fix/multiline-toast
Open

fix: render safeHTML in toasts#769
RitvikSardana wants to merge 1 commit into
frappe:mainfrom
RitvikSardana:fix/multiline-toast

Conversation

@RitvikSardana

@RitvikSardana RitvikSardana commented Jun 9, 2026

Copy link
Copy Markdown
Member

1. Render safe HTML inside the toast

Backend-supplied HTML was showing up as escaped text instead of rendering. We now sanitize the message with DOMPurify (already in package.json) and render it — only the allow-listed tags survive, everything else (scripts, event handlers, unknown tags) is stripped.

How it works:

const ALLOWED_TAGS = ['a', 'em', 'strong', 'i', 'b', 'u']

1. toast.success('<b>Saved</b>')  message comes in as a string
2. DOMPurify.sanitize(message, { ALLOWED_TAGS })  strip unsafe tags
3. () => h('span', { innerHTML: cleanHTML })  wrap as a render fn ( v-html)
4. Pass the render fn to vue-sonner as the title
5. Sonner's Toast.vue renders it via <component :is="toast.title" /> 

2. Fix the collapsed state with mixed toast heights

When two toasts were stacked in the collapsed state — one multiline, one single-line — the taller toast's content spilled out the top of the card and looked broken.

vue-sonner clamps every non-front toast to the front toast's height and hides their content, but only on [data-styled] toasts — and our unstyled config strips that attribute. We re-add the rule so collapsed back toasts fade their content out, matching sonner's intended stacked look.

Image Image

closes: #756

Coverage: 56.93% (+0.04% vs main)

@netchampfaris

Copy link
Copy Markdown
Contributor

/barista review does the sanitize test need to be in a separate file?

@barista-for-frappe

barista-for-frappe Bot commented Jun 9, 2026

Copy link
Copy Markdown

Re-reviewing per @netchampfaris — focused on whether the sanitize test needs its own file.

Short answer: no, it doesn't have to be separate — but the split is reasonable.

  • Vitest picks the environment per file via the @vitest-environment docblock; you can't mix node and jsdom in one file. The only reason Toast.sanitize.test.ts exists is that Toast.test.ts is pinned to node, and DOMPurify needs a DOM.
  • Alternative: set Toast.test.ts to @vitest-environment jsdom, use the real DOMPurify everywhere, and drop the passthrough stub at Toast.test.ts:29. That collapses it to one file with no stub. jsdom would run the existing API-surface tests fine too.
  • Keeping the split is also fine — it isolates the one DOM-dependent test and leaves the rest in the faster node env. Its only cost is the stub, which is harmless here since nothing in Toast.test.ts asserts sanitization.

So it's a judgment call, not a correctness issue. I'd lean toward whichever the repo already does elsewhere for consistency.

Looks good on the fix itself:

  • toast.ts:18 — sanitizing the title and rendering via innerHTML is the right way to handle backend HTML; the ALLOWED_TAGS allow-list is tight and the XSS test in Toast.sanitize.test.ts:38 covers script/onerror stripping.
  • description (legacy text) stays a plain string and sonner escapes it — good, only the title path goes through innerHTML.
  • No new public props/slots/events — the success/error/warning/info wrappers just make explicit what Object.assign already exposed, with the same string | Component | VNode types.
  • One minor thing worth a glance: every string title now becomes <span innerHTML> instead of sonner's plain-text title. Screenshots show it rendering fine, but if sonner applies any title-specific truncation/styling to text vs vnode titles, double-check a long single-line toast still clamps as before.

barista · claude-opus-4-8 · 8.3k in / 4.7k out · 555k cached · 75s · $0.485

@RitvikSardana

Copy link
Copy Markdown
Member Author

@netchampfaris
Different files seem fine, right?
If yes, we can merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants